Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Jest tests and publish automation with changesets #9

Closed
wants to merge 1 commit into from

Conversation

robmosca
Copy link

@robmosca robmosca commented Sep 11, 2023

This PR addresses issues:

In particular:

  • I added Jest tests covering 100% of the code
  • I included automation for running Jest tests
  • I included a command for type checking with Typescript and fixed a minor type problem with the window object
  • I included changesets
  • I included automation of deployments through changesets and the changesets release action for GitHub

For this to work it is necessary that:

  1. The GITHUB_TOKEN is granted permissions to create and approve pull requests. This can be done in Settings -> Actions -> General -> Workflow permissions -> Allow GitHub Actions to create and approve pull requests
  2. The maintainers create an NPM_TOKEN secret with an NPM token with write permissions to the organization (or at least the corresponding package, for publishing).

See single-spa/single-spa-react#178 for a description of how this works.

The automated pipeline includes:

  • running the tests
  • checking the format
  • checking the types
  • running the linter

Current coverage:
Screenshot 2023-09-11 at 17 02 41

@@ -31,7 +31,7 @@ injectorImportMaps.forEach((scriptEl) => {
return r.json();
} else {
throw Error(
`${errPrefix} import map at url '${scriptEl.src}' must respond with a success HTTP status, but responded with HTTP ${r.status} ${r.statusText}`,
`${errPrefix} Import map at url '${scriptEl.src}' must respond with a success HTTP status, but responded with HTTP ${r.status} ${r.statusText}`,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To harmonize the casing to the other error messages

@@ -69,8 +69,12 @@ const requiresMicroTick = importMapJsons.some(
(json) => json instanceof Promise,
);

const globalWindow = window as typeof window & {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed by Typescript (otherwise throws a type error)

@@ -112,3 +116,5 @@ function injectImportMap(importMaps: ImportMap[]): void {
finalImportMapScriptEl.textContent = JSON.stringify(finalImportMap);
document.head.appendChild(finalImportMapScriptEl);
}

export {};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed in order to be able to import the module in Jest. Otherwise, it complains the file is not a module.

@robmosca
Copy link
Author

Closed in favour of #10

@robmosca robmosca closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant